refactor: implement fdv2 polling initializer / synchronizer#519
refactor: implement fdv2 polling initializer / synchronizer#519
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| data_interfaces::FDv2SourceResult::TerminalError{ | ||
| MakeError(ErrorKind::kErrorResponse, res.Status(), std::move(msg)), | ||
| false}}; | ||
| } |
There was a problem hiding this comment.
HandlePollResult duplicated across initializer and synchronizer
Low Severity
HandlePollResult (~140 lines), MakeError, the URL-building logic, and the error message constants are substantively identical between polling_initializer.cpp and polling_synchronizer.cpp. Any future bug fix or protocol change would need to be applied to both copies, increasing the risk of divergence.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0996cca. Configure here.
| std::move(**result)}}; | ||
| } | ||
| // Silently skip unknown kinds for forward-compatibility. | ||
| return std::nullopt; |
There was a problem hiding this comment.
This ends up with maybe a slightly different layer responsibility, but I think it is likely fine. I think Java returns it, and then it is discarded a layer up, but we aren't acting on it, so it doesn't really make a difference.
There was a problem hiding this comment.
Actually I am a bit concerned that we may be losing granularity of data source reporting. Though maybe we don't have that concern at the moment.
| FDv2ProtocolHandler::Result FDv2ProtocolHandler::HandleEvent( | ||
| std::string_view event_type, | ||
| boost::json::value const& data) { | ||
| if (event_type == kServerIntent) { |
There was a problem hiding this comment.
It might be nice to have this handle which event type it is in a more compact way, like a switch, and then handle the message is functions. Just makes it easier to see at a glance what is handled independent of how it is handled.
| tl::expected<std::optional<DeleteObject>, JsonError>>(data); | ||
| if (!result) { | ||
| Reset(); | ||
| return FDv2Error{std::nullopt, "could not deserialize delete-object"}; |
There was a problem hiding this comment.
We do need differentiation for malformed data errors, as we typically will restart on those errors versus just potentially logging another error type.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 37fb4c0. Configure here.
| } else { | ||
| promise->set_value( | ||
| FDv2SourceResult{FDv2SourceResult::Timeout{}}); | ||
| } |
There was a problem hiding this comment.
DoPoll missing closed_ check before processing result
Medium Severity
The DoPoll callback checks order[0] == 0 before checking closed_, so when Close() triggers cancellation via cancel_signal_.emit() and the HTTP request cancellation completes first, the callback calls HandlePollResult with a cancelled/error result instead of returning Shutdown. This is inconsistent with DoNext, which correctly checks closed_ first. During shutdown, this can produce a spurious Interrupted error instead of Shutdown.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 37fb4c0. Configure here.
| if (selector.value) { | ||
| url->append("?basis=" + selector.value->state); | ||
| has_query = true; | ||
| } |
There was a problem hiding this comment.
Null optional dereference when URL parsing fails
Medium Severity
network::AppendUrl can return std::nullopt when URL parsing fails (e.g., malformed PollingBaseUrl). After that call, both MakeRequest functions dereference url via url->append(...) without checking for nullopt, causing undefined behavior. The existing FDv1 polling code correctly guards with && url before dereferencing — see polling_data_source.cpp line 40.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 37fb4c0. Configure here.
2 participants
This implements the C++ polling initializer and synchronizer.
The bulk of this code was generated by Claude based on the Java version, but I've gone through it line-by-line, and I think it makes sense. But I'm new to both FDv2 and ASIO, so I could be missing something.
Probably the most controversial part is the decision from the previous PR to use
std::futureand a blocking call. If we decide we need Java-like conditions, or if we need the callers to be non-blocking, we could change these to useasio::deferredinstead. But I don't think these changes require that yet.Note
Medium Risk
Touches core data-sync and networking concurrency paths (ASIO timers, cancellation, blocking waits), so subtle race/timeout/edge-case behavior could affect reliability even though changes are mostly additive.
Overview
Implements FDv2 polling end-to-end: a new
FDv2ProtocolHandleraccumulatesserver-intent/put-object/delete-objectevents and emits a completeFDv2ChangeSetonpayload-transferred, with explicit handling forerror/goodbyeand forward-compatible skipping of unknown kinds.Adds
FDv2PollingInitializer(one-shot, blocking) andFDv2PollingSynchronizer(repeating, blocking) that call the/sdk/pollendpoint, parse the responseeventsarray through the protocol handler, and translate HTTP/status/error cases intoFDv2SourceResult(including 304→kNone, timeouts, and min poll interval enforcement).Refactors
network::AsioRequester::Requestto useboost::asio::async_initiatefor cleaner completion-token handling, and wires the new FDv2 sources/handler into the internal and server SDK CMake builds with new unit tests for protocol handling.Reviewed by Cursor Bugbot for commit 37fb4c0. Bugbot is set up for automated code reviews on this repo. Configure here.